-
Notifications
You must be signed in to change notification settings - Fork 10
Use dapr durabletask go sidecar ci #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use dapr durabletask go sidecar ci #33
Conversation
Signed-off-by: Javier Aliaga <[email protected]>
Signed-off-by: Javier Aliaga <[email protected]>
Signed-off-by: Javier Aliaga <[email protected]>
1769cee to
d7bd353
Compare
Signed-off-by: Javier Aliaga <[email protected]>
d7bd353 to
864f62c
Compare
Signed-off-by: Javier Aliaga <[email protected]>
| ref: upgrade-dockerfile | ||
| path: durabletask-sidecar | ||
|
|
||
| # TODO: Move the sidecar into a central image repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this or are we happy this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javier-aliaga I get the idea.. but it looks like durabletask-go sidecar is not fully compatible with all the features in the java implementation right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, there are things we do not support
Signed-off-by: Javier Aliaga <[email protected]>
| assertThrows(TimeoutException.class, () -> client.waitForInstanceStart(instanceId, Duration.ofSeconds(2))); | ||
| var instanceId = UUID.randomUUID().toString(); | ||
| Thread thread = new Thread(() -> { | ||
| client.scheduleNewOrchestrationInstance(orchestratorName, null, instanceId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this call is blocking until it finishes
|
|
||
| DurableTaskClient client = new DurableTaskGrpcClientBuilder().build(); | ||
| try (worker; client) { | ||
| client.createTaskHub(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javier-aliaga why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same reason as purgeInstance, it is not implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main problem with all this is that in our implementation we do no support all the features the library supports and I am not sure we want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but in the backend implementation what it does is initialize the persistence. https://github.com/dapr/durabletask-go/blob/1cae3eb4b56b1970ea0bfca29015c0e2add24781/backend/sqlite/sqlite.go#L99
|
|
||
| DurableTaskClient client = new DurableTaskGrpcClientBuilder().build(); | ||
| try (worker; client) { | ||
| client.createTaskHub(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javier-aliaga why is this removed?
Signed-off-by: Javier Aliaga <[email protected]>
cicoyle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment then Im mostly good with this PR. Im ok disabling the tests we dont support for now, but can you open issues in the java-sdk and add the durabletask label so we dont forget to come back to it in the future?
|
I added an issue dapr/java-sdk#1438 |
Issue describing the changes in this PR
Move integration test to use durabletask-go as sidecar instead of the actual user one
resolves #issue_for_this_pr
Pull request checklist
CHANGELOG.mdAdditional information
Additional PR information